Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STYLE: Clean-up ByteSwapper, using private SwapBytes helper function #4855

Merged

Conversation

N-Dekker
Copy link
Contributor

Aims to reduce code duplication. Inspired by a suggestion from Bradley (@blowekamp) at https://github.com/InsightSoftwareConsortium/ITK/pull/4841/files#r1751777344

@github-actions github-actions bot added area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming) labels Sep 20, 2024
@github-actions github-actions bot added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label Sep 20, 2024
}
else
{
itkGenericExceptionMacro("Cannot swap number of bytes requested");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a run-time exception for a "constexpr" conditional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Only the condition of an if constexpr (condition) { ... } must be evaluated at compile-time. Anything inside the if or else clause may be evaluated at run-time.

Copy link
Contributor Author

@N-Dekker N-Dekker Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this exception, we might also just trigger a compile-time assert failure (static_assert). But obviously, that would change the behavior of ByteSwapper (at least for an unsupported number of bytes). It might be considered an improvement, to replace the exception with a static_assert, of course. Would you like that?

We might just do:

static_assert(numberOfBytes == 2 || numberOfBytes == 4 || numberOfBytes == 8);

Would you like that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think it would be an improvement and help detect errors in code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly it would be fine to me to also support other number of bytes. Why not support numberOfBytes == 16, for example? For example, when a user wants to swap 128-bit integers...!

I think the original itkGenericExceptionMacro("Cannot swap number of bytes requested") was there just because we only had three of those helper functions: Swap2, Swap4, and Swap8. Instead, we can now support just any number of bytes! Would you like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still broken, now it's itkVTKPolyDataMeshIO, at https://open.cdash.org/viewBuildError.php?type=1&buildid=9915657:

in instantiation of function template specialization 'itk::VTKPolyDataMeshIO::ReadPointsBufferAsBINARY' requested here

  CASE_INVOKE_BY_TYPE(ReadPointsBufferAsBINARY, inputFile)
                      ^

So would you also want us to drop long double support from itk::VTKPolyDataMeshIO?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsurprisingly, this endian swapping only affects IO classes.

As the other float types are being swapped, why not swap long double as well? Just add size of the long double to the list of the allowed sizes in the assert? @blowekamp what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quick google said 128-bit float may be an abnormal case:
https://www.talospace.com/2018/12/the-saga-of-power-isa-128-bit-long.html

So... Just leave it as a run time exception of now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be fine to me to restore the itkGenericExceptionMacro("Cannot swap number of bytes requested") that I originally proposed, and remove the compile-time static_assert(numberOfBytes is 2 or 4 or 8).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should leave support for long double if there is no need for endian swapping (the usual case).

@blowekamp
Copy link
Member

Looking like a nice cleanup!

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good even as-is.

@github-actions github-actions bot added the area:IO Issues affecting the IO module label Sep 20, 2024
@github-actions github-actions bot removed the area:IO Issues affecting the IO module label Sep 22, 2024
@N-Dekker N-Dekker marked this pull request as ready for review September 23, 2024 08:56
@N-Dekker
Copy link
Contributor Author

Ready! We may of course reconsider/revisit long double support later, as well as a compile-time static_assert on the size or type of the byte-swapped argument. This is just a STYLE pull request anyway.

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@N-Dekker
Copy link
Contributor Author

Great!

@thewtex Thanks, Matt! Please feel free to merge it!

@dzenanz dzenanz merged commit 3b04ad1 into InsightSoftwareConsortium:master Sep 26, 2024
13 checks passed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 1, 2024
Aims to reduce code duplication.

- Follow-up to pull request InsightSoftwareConsortium#4855
commit 3b04ad1
"STYLE: Clean-up ByteSwapper, using private `SwapBytes` helper function"
dzenanz pushed a commit that referenced this pull request Oct 3, 2024
Aims to reduce code duplication.

- Follow-up to pull request #4855
commit 3b04ad1
"STYLE: Clean-up ByteSwapper, using private `SwapBytes` helper function"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 3, 2024
These nine `ByteSwapper` member functions were only for internal use. They are
replaced now with private member functions `SwapBytes` and `SwapWriteRange`, by:
- pull request InsightSoftwareConsortium#4855
commit 3b04ad1, and
- pull request InsightSoftwareConsortium#4862
commit 9413dc4
dzenanz pushed a commit that referenced this pull request Oct 7, 2024
These nine `ByteSwapper` member functions were only for internal use. They are
replaced now with private member functions `SwapBytes` and `SwapWriteRange`, by:
- pull request #4855
commit 3b04ad1, and
- pull request #4862
commit 9413dc4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Style Style changes: no logic impact (indentation, comments, naming)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants